feat: introduce flag validate_user_defined_fields#352
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
- Coverage 91.72% 91.29% -0.43%
==========================================
Files 94 94
Lines 10038 10372 +334
==========================================
+ Hits 9207 9469 +262
- Misses 831 903 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Vlad-Bukatin
force-pushed
the
ignore_user_defined_fields
branch
from
579d0cf to
0b8f197
Compare
|
@Vlad-Bukatin sorry, I'll get to this as soon as I can. I just have quite a lot going on, and this change is a bit larger so I want to give it a proper read. |
davidsteiner
left a comment
davidsteiner
left a comment
There was a problem hiding this comment.
The general direction looks good to me. My only functional concern is how we check the parent groups - left a comment on that.
Also, we're not too strict on test coverage, but I feel like builder.rs has a few interesting branches which aren't covered by any tests. It would be great to add some coverage for those.
Apologies for the very slow review.
| .next_field() | ||
| .ok_or(ParserError::Malformed("incomplete group".to_string()))? | ||
| }; | ||
| } else if Self::parent_contains_tag(parent, current_tag) |
There was a problem hiding this comment.
If I read this correctly, this checks only the immediate parent. I think this is an issue for any scenario where the next tag isn't one for the enclosing group, but another ancestor - e.g. when an inner group and an outer group end at the same tag, so the next tag is for the message (the outer group's parent). What do you think?
|
@davidsteiner Thanks for the review! No worries at all about the delay - I've been quite busy on my end as well. Hopefully I would be able to do it next week |
2 participants
closes: #331